Make transactional email footer settings links clickable - #2619
Make transactional email footer settings links clickable#2619timmilazzo wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
builder-io-integration
Bot
changed the title
This comment has been minimized.
This comment has been minimized.
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🟡
Review Details
Code Review Summary
PR #2619 adds linked footer tokens to the shared email renderer, routes core notification emails through the branded renderer, and introduces Plan comment-email preferences with an optimistic settings toggle. The overall architecture is sensible: HTML and text email parts preserve the destination, notification metadata only advertises opt-out controls when a sender supplies them, and Plan recipients are filtered through the shared activity-preference resolver. This is a standard-risk change because it spans notification behavior, persistence, and several app URL surfaces.
Key Findings
🟡 MEDIUM: Plan preference saves can race, and failed loads are presented as an apparently enabled default; see inline comments. 🟡 MEDIUM: New Analytics and Assets email URLs bypass their apps' configured base paths, so mounted deployments can receive links to the host root instead of the app.
The added unit coverage is useful for footer rendering and recipient filtering, and the implementation correctly escapes footer content and link attributes. The dev server was healthy, but browser verification could not execute because the browser-test executors had no Chrome MCP tools available.
🧪 Browser testing: Could not verify — browser automation unavailable in the executor environment; all planned UI cases were escalated.
| const save = useCallback( | ||
| async (patch: PlanUserPrefs) => { | ||
| const previous = prefs; | ||
| setPrefs((current) => ({ ...current, ...patch })); | ||
| const res = await fetch(agentNativePath(PREFS_PATH), { |
There was a problem hiding this comment.
🟡 Serialize preference saves to preserve the latest toggle
Each toggle starts an independent PUT, so a rapid off-then-on sequence can complete out of order and persist the older value while the optimistic UI shows the newer one. Serialize/coalesce saves or add a revision/order check so the last user choice is authoritative; the rollback also needs to be scoped to the request that failed.
Additional Info
Found by 2 of 3 review agents; confirmed from the hook's independent fetch calls and the server's merge-only PUT behavior.
| useEffect(() => { | ||
| let cancelled = false; | ||
| void (async () => { | ||
| try { | ||
| const res = await fetch(agentNativePath(PREFS_PATH)); | ||
| const json = res.ok ? await res.json() : null; |
There was a problem hiding this comment.
🟡 Surface preference-load failures instead of treating them as defaults
A rejected fetch or any non-2xx response falls through to {} and clears loading, while the settings switch interprets the missing value as enabled. This makes an unreadable preference store indistinguishable from an unset preference and lets the user believe the toggle is authoritative; expose the load error and keep the control disabled or visibly failed until the read succeeds.
Additional Info
Found by 1 of 3 review agents; confirmed by the hook and settings route, which only consumes loading/prefs.
| export function analyticsUrl(path: string): string { | ||
| const base = getAppProductionUrl().replace(/\/+$/, ""); | ||
| return `${base}${path.startsWith("/") ? path : `/${path}`}`; |
There was a problem hiding this comment.
🟡 Preserve Analytics base paths in notification URLs
analyticsUrl appends paths directly to the production origin, but Analytics routes strip a configured APP_BASE_PATH/VITE_APP_BASE_PATH. In a deployment mounted at /analytics, new alert, monitor, error, and report links point to /settings or /monitoring at the host root instead of the Analytics app. Use the configured base-path-aware URL helper and cover a mounted-path case.
Additional Info
Found by 1 of 3 review agents; surrounding Analytics routing reads APP_BASE_PATH/VITE_APP_BASE_PATH, while this helper does not.
| emailFooter: | ||
| "You received this because email notifications are on in your {link}.", | ||
| emailFooterLinkLabel: "Assets settings", | ||
| emailFooterLinkUrl: `${getAppProductionUrl().replace(/\/+$/, "")}/settings`, |
There was a problem hiding this comment.
🟡 Use the Assets base-path-aware URL builder
This direct concatenation bypasses Assets' existing absoluteUrl helper, which prefixes getConfiguredAppBasePath(). When Assets is mounted under /assets, the generation email footer links to the host /settings rather than /assets/settings; build the URL through the existing helper and add a mounted-path test.
Additional Info
Found by 1 of 3 review agents; confirmed by templates/assets/server/lib/json.ts and its base-path tests.
2 participants
Summary
Adds a
footerLinkcapability torenderEmailso footer text can include a real, clickable link (e.g. to a settings page), and wires this through the core notifications channel and the Plan/Slides templates so "turn these off" copy actually links somewhere.Problem
Transactional emails referenced "Settings" in the footer as plain, non-clickable text, and env-configured ops notification recipients (who have no per-user opt-out toggle) were sometimes told to go turn one off anyway. There was also no consistent way for a notification sender to declare which settings surface controls a given email type.
Solution
renderEmailnow supports a{link}token infooterthat is replaced with a real anchor (HTML) orlabel (url)(plain text) when afooterLinkis supplied, and left untouched otherwise. The core notification email channel builds this footer from per-notification metadata (emailFooter,emailFooterLinkLabel,emailFooterLinkUrl), only adding an opt-out footer when the sender explicitly names one — so ops recipients without a toggle never get told to disable it. The Plan and Slides templates were updated to use this footer link for their comment-notification emails and to route through their own settings pages, with Plan gaining a dedicated user-preferences endpoint and opt-out filtering for comment notifications.Key Changes
renderEmail(email-template.ts): newfooterLinkarg andresolveFooterLinkhelper that substitutes a{link}token with an anchor in HTML andlabel (url)in plain text, or leaves the token visible if no link is provided.channels.ts): notification emails now render viarenderEmailinstead of bare paragraphs, picking up shared branding; addednotificationEmailFooterto build the footer/footerLink only whenmetadata.emailFooteris set, keeping ops-only notifications free of misleading opt-out text.comment-notifications.tsadds asettingsLink()helper and appends a "Turn these off in {link}." footer with a link to Plan settings; introducesPLAN_USER_PREFS_KEYshared constant, newuser-prefs.get.ts/user-prefs.put.tsroutes for reading/writing per-user preferences, and filters comment recipients throughresolveActivityRecipientsso opted-out users are skipped.comment-notifications.tsnow uses a{link}footer token withgetSettingsUrl()(new helper in_app-url.ts) instead of a static "Slides settings" string, plus a changelog entry documenting the clickable settings link.renderEmailfooterLinkAPI and the updated notification email behavior.To clone this PR locally use the Github CLI with command
gh pr checkout 2619You can tag me at @BuilderIO for anything you want me to fix or change